Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent crashing when mv warns and report warnings to the user instead. #1326

Merged
merged 1 commit into from
Sep 15, 2016

Conversation

ddeboer
Copy link
Contributor

@ddeboer ddeboer commented Sep 14, 2016

Fix #1324.

In some cases, mv will throw a warning, while still moving the files correctly and returning a 0 return code:

"mv: can't preserve ownership of ... Permission denied".

@ddeboer ddeboer force-pushed the fix-preserve-ownership branch 2 times, most recently from 6bfbba8 to b8d80fd Compare September 14, 2016 14:38
@ddeboer
Copy link
Contributor Author

ddeboer commented Sep 14, 2016

Should there be a test for this as well? Not sure how I can mock rebar_utils:sh/2.

{ok, []} ->
ok;
{ok, Warning} ->
?WARN("mv warning ~p", [Warning]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the string to "mv: ~p" there.

@ferd
Copy link
Collaborator

ferd commented Sep 14, 2016

A test would be good. We may not need to mock anything -- if we can make a file inside CT's priv_dir argument, and give it permissions that would be lost it would be good. If we depend on a specific environment for this to work (i.e. multiple mount points), then yeah we'd need to mock otherwise and skip tests and so on.

To mock rebar_utils:sh/2 I'd probably just:

meck:new(rebar_utils, [passthrough]),
meck:expect(rebar_utils, sh, fun(<STRING_FOR_MV>, _) -> {ok, "Warning"}  end),
ok = rebar_utils:mv(Src, Dest),
meck:unload(rebar_utils).

We possibly do not need to test that the warning is being relayed, though that would be nicer and would require more mocking (and conditional test skips for windows runs).

@ddeboer ddeboer force-pushed the fix-preserve-ownership branch 2 times, most recently from 905a5a1 to 8457428 Compare September 14, 2016 15:26
@ddeboer
Copy link
Contributor Author

ddeboer commented Sep 14, 2016

If we depend on a specific environment for this to work (i.e. multiple mount points), then yeah we'd need to mock otherwise and skip tests and so on.

Added a test. Mocking, because I wasn’t able to reproduce mv throwing a warning when transferring within one filesystem.

@ddeboer ddeboer force-pushed the fix-preserve-ownership branch from 8457428 to e2c57e4 Compare September 14, 2016 16:01
In some cases, mv will throw a warning, while still moving the files
correctly and returning a 0 return code:

"mv: can't preserve ownership of ... Permission denied".
@ddeboer ddeboer force-pushed the fix-preserve-ownership branch from e2c57e4 to 2b6fa7a Compare September 15, 2016 06:57
@ddeboer
Copy link
Contributor Author

ddeboer commented Sep 15, 2016

And now green. 😉

@ferd ferd changed the title Ignore mv warnings Prevent crashing when mv warns and report warnings to the user instead. Sep 15, 2016
@ferd
Copy link
Collaborator

ferd commented Sep 15, 2016

Merging, thanks!

@ferd ferd merged commit 6a8150e into erlang:master Sep 15, 2016
@ddeboer
Copy link
Contributor Author

ddeboer commented Sep 15, 2016

Thank you for the help, Fred!

@ddeboer ddeboer deleted the fix-preserve-ownership branch September 15, 2016 12:54
@ddeboer ddeboer mentioned this pull request Sep 20, 2016
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
ddeboer added a commit to zotonic/zotonic that referenced this pull request Sep 21, 2016
* Switch to recent rebar3 build for erlang/rebar3#1326
* Add docker-compose.yml
* Update docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants